Skip to content

Conversation

@palfrey
Copy link
Member

@palfrey palfrey commented Apr 5, 2025

Description

Right now, if you run nix run -L .#nativelink -- ./nativelink-config/examples/basic_cas.json5 you get

thread 'main' panicked at src/bin/nativelink.rs:672:31:
Path segments must not start with `:`. For capture groups, use `{capture}`. If you meant to literally match a segment starting with a colon, call `without_v07_checks` on the router.

because #1674 upgraded axum to 0.8.x and they changed the router syntax, but there's no tests that run a config with the admin enabled. #1646 does do this, and hence a) why I've hit it and b) why I haven't added a test yet, as that PR will do the testing.

Type of change

Please delete options that aren't relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

nix run -L .#nativelink -- ./nativelink-config/examples/basic_cas.json5 from current main

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link
Contributor

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

+@jaroeichler

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-cc / ubuntu-24.04, Local / lre-rs / macos-15, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, pre-commit-checks, ubuntu-24.04, ubuntu-24.04 / stable, windows-2022 / stable (waiting on @jaroeichler)

Copy link
Contributor

@jaroeichler jaroeichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained, and all files reviewed

@jaroeichler jaroeichler merged commit 3d8f4a8 into TraceMachina:main Apr 5, 2025
34 checks passed
@palfrey palfrey deleted the admin-router-syntax branch April 5, 2025 16:49
path,
Router::new().route(
"/scheduler/:instance_name/set_drain_worker/:worker_id/:is_draining",
"/scheduler/{instance_name}/set_drain_worker/{worker_id}/{is_draining}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEAMers

MarcusSorealheis pushed a commit to MarcusSorealheis/nativelink that referenced this pull request Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants